Skip to content

refactor: split main otel config into multiple connections, add tests #186

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Apr 10, 2025

Conversation

obs-gh-mattcotter
Copy link
Collaborator

@obs-gh-mattcotter obs-gh-mattcotter commented Apr 7, 2025

Description

OB-36266: Split main otel config into multiple connections. Use the agent config object as the source of truth instead of viper. Add tests.

I also tested this with the new observe-agent config --render-otel command. I enabled all options in my agent config, and diffed the output between main and this branch and only these lines changed:

104c104
<             - /usr/local/observe-agent/config/otel-collector.yaml
---
>             - /usr/local/observe-agent/connections/common/base.yaml.tmpl
123c123
<         include: /usr/local/observe-agent/config/otel-collector.yaml
---
>         include: /usr/local/observe-agent/connections/common/base.yaml.tmpl

This is expected since the otel-collector.yaml file no longer exists.

Checklist

  • Created tests which fail without the change (if possible)

@obs-gh-mattcotter obs-gh-mattcotter force-pushed the OB-36266-config-refactor branch 2 times, most recently from 5e95cdb to cd22dc3 Compare April 7, 2025 16:35

var configYaml, configMapstructure config.AgentConfig

// Decode via mapstrucutre (which is how viper does it)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit typo mapstructure

},
[]CollectorConfigFragment{
{
enabledCheck: func(agentConfig *config.AgentConfig) bool {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these are basically the shared components between other connection types right? so if we add more connection types we'd need to add them to this check here too? any reason to not just always include this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's right. I was thinking it would be nice to have a total noop mode (ie operate exactly like a vanilla otelcol), but it's probably not worth the extra complexity here. I changed this to always return true.

},
{
enabledCheck: func(agentConfig *config.AgentConfig) bool {
return agentConfig.Forwarding.Enabled
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

before there was no value here right? so if we change this to be default off, that's a breaking change I think. Same for the healthcheck below. do these default to on?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These do default to on, but it happens via defaulting the Enabled variable to true in the config definition. I added "default" struct tags to config.AgentConfig and the go-defaults library to set them. The new TestAgentConfigFromViper validates that this works as intended.

@obs-gh-mattcotter obs-gh-mattcotter force-pushed the OB-36266-config-refactor branch from f9fce4b to 0e7e577 Compare April 10, 2025 15:11
@obs-gh-mattcotter obs-gh-mattcotter merged commit 97fbd00 into main Apr 10, 2025
8 checks passed
@obs-gh-mattcotter obs-gh-mattcotter deleted the OB-36266-config-refactor branch April 10, 2025 15:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants